Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Append newline to end of extensionHeaders if necessary #1317

Closed
wants to merge 2 commits into from

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented May 14, 2016

Fixes #1316.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 14, 2016
@stephenplusplus
Copy link
Contributor

Thank you for this! What do you think about accepting an object for headers, e.g.

file.getSignedUrl({
  extensionHeaders: {
    'x-goog-acl': 'public-read',
    'x-goog-meta-foo': 'bar'
  },
  // ...

File.prototype.getSignedUrl = function(options, callback) {
  ...

  var extensionHeadersString;

  if (options.extensionHeaders) {
    for (var headerName in options.extensionHeaders) {
      extensionHeadersString += format('{name}:{value}\n', {
        name: headerName,
        value: options.extensionHeaders[headerName]
      });
    }
  }

  ...
};

@stephenplusplus stephenplusplus added the api: storage Issues related to the Cloud Storage API. label May 14, 2016
@aknuds1
Copy link
Contributor Author

aknuds1 commented May 14, 2016

@stephenplusplus I also thought that an object would be better, but I thought about maintaining backwards compatibility and that it would be easier to fix the existing interface (extension headers represented by a string) if it's supposed to be supported in the future.

If backwards compatibility doesn't matter, I'd definitely prefer representing extension headers just as an object.

@stephenplusplus
Copy link
Contributor

Thank you for thinking about the backwards compatibility issue, but it's okay to break it if we've discovered an easier API. We will just bump the minor to follow along with semver.

@aknuds1
Copy link
Contributor Author

aknuds1 commented May 14, 2016

Do you want me to update the PR to represent extension headers as an object then, @stephenplusplus?

@stephenplusplus
Copy link
Contributor

If you're up for it, that would be great 👍

@aknuds1
Copy link
Contributor Author

aknuds1 commented May 14, 2016

Will do then.

@aknuds1
Copy link
Contributor Author

aknuds1 commented May 14, 2016

@stephenplusplus Done!

@stephenplusplus
Copy link
Contributor

Merged in via 17e510b. Thanks again @aknuds1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants